portability: cmsis: Fix possible race in osEventFlagsWait() #97123
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
zephyrbot
added
area: CMSIS API Layer
|
Maybe we should even replace |
|
Cc: @ckhardin |
|
probably looking for something maybe closer to this diff --git a/subsys/portability/cmsis_rtos_v2/event_flags.c b/subsys/portability/cmsis_rtos_v2/event_flags.c
index fc3d50f067d..86dbc2846cd 100644
--- a/subsys/portability/cmsis_rtos_v2/event_flags.c
+++ b/subsys/portability/cmsis_rtos_v2/event_flags.c
@@ -79,7 +79,7 @@ uint32_t osEventFlagsClear(osEventFlagsId_t ef_id, uint32_t flags)
}
rv = k_event_test(&events->z_event, 0xFFFFFFFF);
- k_event_clear(&events->z_event, flags);
+ k_event_clear(&events->z_event, flags & rv);
return rv;
}
@@ -115,13 +115,21 @@ uint32_t osEventFlagsWait(osEventFlagsId_t ef_id, uint32_t flags, uint32_t optio
}
if ((options & osFlagsWaitAll) != 0) {
- rv = k_event_wait_all(&events->z_event, flags, false, event_timeout);
+ if ((options & osFlagsNoClear) != 0) {
+ rv = k_event_wait_all(&events->z_event, flags,
+ false, event_timeout);
+ } else {
+ rv = k_event_wait_all_safe(&events->z_event, flags,
+ false, event_timeout);
+ }
} else {
- rv = k_event_wait(&events->z_event, flags, false, event_timeout);
- }
-
- if ((options & osFlagsNoClear) == 0) {
- k_event_clear(&events->z_event, flags);
+ if ((options & osFlagsNoClear) != 0) {
+ rv = k_event_wait(&events->z_event, flags,
+ false, event_timeout);
+ } else {
+ rv = k_event_wait_safe(&events->z_event, flags,
+ false, event_timeout);
+ }
}
if (rv != 0U) { |
jerome-pouiller
force-pushed
the
osEventFlagsWait-race
branch
2 times, most recently
from
7f402d2 to
387b829
Compare
|
v2:
|
jerome-pouiller
force-pushed
the
osEventFlagsWait-race
branch
from
09ffe9b to
f3bde80
Compare
zephyrbot
added
the
area: Tests
|
v3:
@rgundi, I believe you are the author of the comment in |
In osEventFlagsWait(), if an event raises after the call to k_event_wait() and before the call to k_event_clear(), we may clear it while it is not going to be report to the caller. Reported-by: Rahul Gurram <[email protected]> Signed-off-by: Jérôme Pouiller <[email protected]>
The CMSIS-RTOS specification says "The function returns the event flags
before clearing".
In the original code, if another thread set an event between k_event_test()
and k_event_clear(), there was a risk the function clear a flag without
reporting it to the caller:
T1 T2
k_event_test(..) == 0 ...
... k_event_post(.. 1)
k_event_clear(.. 1) ...
return 0 (while event 1 has been cleared)
Signed-off-by: Jérôme Pouiller <[email protected]>
jerome-pouiller
force-pushed
the
osEventFlagsWait-race
branch
from
f3bde80 to
4dc7470
Compare
The CMSIS-RTOS specification says "The function returns the event flags stored in the event control block". In the original code, osEventFlagsSet() called k_event_post() and then k_event_test(). It worked mostly fine if the thread has higher priority than the waiter thread. In the opposite case, the event was not reported to the user. With the last changes, the waiter thread use k_event_wait_safe(). So, the event is posted and consumed in an atomic way. So, the issue above happen even if the waiter thread has a lower priority then the poster. This patch fixes the both cases. Signed-off-by: Jérôme Pouiller <[email protected]>
Until now, the main thread preempted thread2 between k_event_post() and k_event_test(). Then main thread consumed the event before it was tested by thread2. This behavior was not correct since the caller can't know the event is in fact consumed (the CMSIS-RTOS specification says osEventFlagsSet() "returns the event flags stored in the event") This behavior is now fixed. So we can fix the test. Signed-off-by: Jérôme Pouiller <[email protected]>
jerome-pouiller
force-pushed
the
osEventFlagsWait-race
branch
from
4dc7470 to
8d93d75
Compare
|
|
@ckhardin, what do think about the v3? |
jhedberg
approved these changes
Oct 14, 2025
ckhardin
approved these changes
Oct 14, 2025
looks good - the switch statement makes the control flow a lot easier to follow then the nested if's - so 👍 |
nashif
approved these changes
Oct 14, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In osEventFlagsWait(), if an event raises after the call to k_event_wait() and before the call to k_event_clear(), we may clear it while it is not going to be report to the caller.
Reported-by: Rahul Gurram [email protected]